Skip to content

Update atomicfu to 0.18.5 #3501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Update atomicfu to 0.18.5 #3501

merged 2 commits into from
Nov 15, 2022

Conversation

mvicsokolova
Copy link
Contributor

Apply JVM IR compiler plugin

@mvicsokolova mvicsokolova changed the base branch from master to develop October 25, 2022 14:42
@mvicsokolova mvicsokolova requested a review from qwwdfsad October 25, 2022 14:42
@mvicsokolova
Copy link
Contributor Author

mvicsokolova commented Oct 25, 2022

A very dumb problem ocurred:

:kotlinx-coroutines-core:jvmApiCheck in this build failed because atomicfu JVM IR compiler plugin generated some declarations with PUBLIC visibility modifier, and those appeared in the public api, that is obviously not right.

For some reason this simple bug was not revealed by the test build of coroutines with atomicfu JVM IR plugin applied. The config of the build is equal to the train config, actually, but seems that :kotlinx-coroutines-core:jvmApiCheck was not called there at all, I could not find it in the build log.

I filed this issue in kotlinx-atomicfu: Kotlin/kotlinx-atomicfu#258
And prepared a fix for this problem for Kotlin 1.7.21, 1.8.0 and master: https://jetbrains.team/p/kt/reviews/7497/timeline
UPD: the fix is merged

@mvicsokolova
Copy link
Contributor Author

mvicsokolova commented Oct 25, 2022

That's too bad, and really reveals the problem of the lack of atomicfu tests and our test train builds ☹️. That code in the atomicfu JVM IR compiler plugin is very old, but we never ran into this issue with visibility modifiers.

I probably need to add some tests to atomicfu-gradle-plugin that would also dump the public api and check that no generated declarations leak

@qwwdfsad
Copy link
Collaborator

I see that apiCheck is explicitly disabled in these train branches which probably should not be the case

@mvicsokolova
Copy link
Contributor Author

Yes, apiCheck and knitCheck tasks were always excluded in the train builds 😞. I've asked to turn them on.

And I turned on the apiCheck in that test build, now it fails at jvmApiCheck as it should have :(

@qwwdfsad
Copy link
Collaborator

@mvicsokolova is it safe to merge or we do wait for something else?

@mvicsokolova
Copy link
Contributor Author

Yes I'll merge it. I just forgot to press the button.

@mvicsokolova mvicsokolova merged commit 30e905c into develop Nov 15, 2022
@mvicsokolova mvicsokolova deleted the update-atomicfu branch November 15, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants